Conversation
HansUweRempler
added
Epic: Continuous integration
elbosch
left a comment
elbosch
left a comment
There was a problem hiding this comment.
let us talk about naming and coding conventions.
core/testing/include/Kiso_Testing.h
Outdated
| */ | ||
| enum KISO_TESTING_Retcodes_E | ||
| { | ||
| TEST_RETCODE_TEST_SUITE_ALREADY_REGISTERED = RETCODE_FIRST_CUSTOM_CODE, |
There was a problem hiding this comment.
so far we had started every retcode with RETCODE_ so in this case it would be like RETCODE_TESTING
There was a problem hiding this comment.
to be honest I did not change in the naming except if there is something clear. I will change it
| /** | ||
| * @brief Encapsulates the elements composing a message.. | ||
| */ | ||
| typedef struct CCMsg_S |
There was a problem hiding this comment.
i would expect this to be somewhere defined in a CChannel, CCMessage, etc. context
There was a problem hiding this comment.
Yes, we should match maybe more the names used in python
There was a problem hiding this comment.
as you can see below there are functions defined here depending on this structure, do we really need to hide it or to add indirection here?
There was a problem hiding this comment.
The question actually is: Why do those functions depend on this structure?
Looks to me that the name is not properly chosen. Are we more likely talking about a testing context?
What do those functions really expect?
Why should this Testing API depend on any detail on how things are stored, assembled,
core/testing/include/Kiso_Testing.h
Outdated
| /** | ||
| * @brief Defines a prototype type for the setup functions of the test suites and test cases. | ||
| */ | ||
| typedef Retcode_T (*StpFct_T)(CCMsg_T *ccmsg); |
There was a problem hiding this comment.
we need to talk about the naming conventions.. First rule in my opinion it should be pronounceable and clear.
here it could be StopFunction or a StepFunction. well established abbreviations are ok.
sebastianpfischer
left a comment
sebastianpfischer
left a comment
There was a problem hiding this comment.
I think we need to put it within an app / example place or root/testing/*
|
@khalifima are you able to work on this this week or shall we support here? |
| @@ -0,0 +1,192 @@ | |||
| /********************************************************************************************************************** | |||
There was a problem hiding this comment.
We should discuss about the location of testing
There was a problem hiding this comment.
testing is a shared library between the test cases written in e.g core/essentials or boards and the test application. I think this is the place where it has to be located
There was a problem hiding this comment.
I'm unfortunately not convinced. I still believe that high cohesion is important, we need to discuss it the 3 of us.
|
@khalifima can you please update here? We need this for the planned release v0.1 on 13.03.20. Please note that if you can't continue right now here, @elbosch is volunteering to take over the task. |
| /** | ||
| * @brief Encapsulates the elements composing a message.. | ||
| */ | ||
| typedef struct CCMsg_S |
There was a problem hiding this comment.
The question actually is: Why do those functions depend on this structure?
Looks to me that the name is not properly chosen. Are we more likely talking about a testing context?
What do those functions really expect?
Why should this Testing API depend on any detail on how things are stored, assembled,
| /** | ||
| * @brief Enumerates the types of messages exchanged between the Test_Executor and the Test_Controller | ||
| */ | ||
| enum CCMsg_MessageType_E |
There was a problem hiding this comment.
from naming perspective this does not belong here.
| /** | ||
| * @brief Encapsulates the elements composing the message header. | ||
| */ | ||
| typedef struct MessageHeader_S |
There was a problem hiding this comment.
This name is not properly showing it's scope.
| */ | ||
| typedef struct MessageHeader_S | ||
| { | ||
| uint8_t messageInfo; /**< version is 2 bits, message type is 2 bits and the remaining 4 bits are reserved */ |
There was a problem hiding this comment.
do we need this level of embedded squeezing of information?
| typedef struct MessageHeader_S | ||
| { | ||
| uint8_t messageInfo; /**< version is 2 bits, message type is 2 bits and the remaining 4 bits are reserved */ | ||
| uint8_t messageToken; |
There was a problem hiding this comment.
could you please also describe the content of those members
| #endif | ||
|
|
||
| #ifndef CCHANNEL_MAX_NUMBER_OF_TLV_ELEMENTS | ||
| #define CCHANNEL_MAX_NUMBER_OF_TLV_ELEMENTS 2 |
There was a problem hiding this comment.
if this is configurable in this context, it should start with TEST
| } | ||
| } | ||
|
|
||
| static uint16_t calculateCRC(uint8_t *buffer, uint8_t length) |
There was a problem hiding this comment.
can't we just use CRC from utils?
| SetupFct_T setup; | ||
| TearDownFct_T teardown; | ||
| TstSte_T testSuites[TEST_MAX_NUMBER_OF_TEST_SUITES]; | ||
| } TstEnt_T, TstEnt_T; |
There was a problem hiding this comment.
Please stick to our coding guidelines here.
| SetupFct_T setup; | ||
| RunFct_T run; | ||
| TearDownFct_T teardown; | ||
| } TstCse_T, TstCse_T; |
There was a problem hiding this comment.
This does not simplify readability. and why twice?
| SetupFct_T setup; | ||
| TearDownFct_T teardown; | ||
| TstCse_T testCases[TEST_MAX_NUMBER_OF_TEST_CASES_PER_TEST_SUITE]; | ||
| } TstSte_T; |
|
@elbosch please finish review and give me sign when you are done so I can work on everything one time |
elbosch
left a comment
elbosch
left a comment
There was a problem hiding this comment.
Please respect our coding guidelines as much as possible.
Scope should be better maintained for some types and resources.
5 participants
the testing library is a shared component between the test application and the test suites therefore a natural place for it is in core/ for a clean dependency structure.